Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding docs for payment metadata #124

Merged
merged 6 commits into from
Feb 29, 2024
Merged

Adding docs for payment metadata #124

merged 6 commits into from
Feb 29, 2024
+845 −0

Conversation

hydra-yse
Copy link
Member

No description provided.

dangeross
dangeross reviewed Jan 18, 2024
View reviewed changes
Copy link
Contributor

@dangeross dangeross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just did a quick pass to help with syntax errors

snippets/csharp/Metadata.cs Outdated Show resolved Hide resolved
snippets/csharp/Metadata.cs Show resolved Hide resolved
snippets/csharp/Metadata.cs Show resolved Hide resolved
dangeross
dangeross reviewed Jan 18, 2024
View reviewed changes
snippets/go/metadata.go Outdated Show resolved Hide resolved
ok300
ok300 reviewed Jan 24, 2024
View reviewed changes
.github/workflows/main.yml Outdated Show resolved Hide resolved
feat: adding docs for payment metadata
41ef4de 
fix: fixing language typos
@hydra-yse hydra-yse force-pushed the payment-metadata branch 4 times, most recently from 4a5b289 to 4871d48 Compare January 25, 2024 14:40
@ok300
Copy link
Contributor

ok300 commented Jan 31, 2024

I merged the latest main, which upgrades the SDK dependency to 0.2.15.

The Swift CI error can be ignored for now IMO, as it will go away with the next release, as described here.

@ok300
Copy link
Contributor

ok300 commented Jan 31, 2024

@hydra-yse a few things I found in the PR:

  • The new payment_metadata.md page is missing an entry in SUMMARY.md. If it's not linked from there (or any other existing page), the html won't be generated.
  • In the 3rd group of snippets, the comment // Note: The following are equivalent is only present for some of the languages.
  • In the 4th group of snippets, the comment // This will work is sometimes referring to a list of equivalent filters. It might make sense to change it to // Any of these will work for those languages, as otherwise it could give the impression that what will work is specifying those two equivalent ways at the same time.

Other than that, LGTM. Good work!

dangeross
dangeross approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@dangeross dangeross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

snippets/rust/src/receive_onchain.rs Outdated Show resolved Hide resolved
@dangeross
Copy link
Contributor

Is there something blocking this PR?

@hydra-yse hydra-yse requested a review from ok300 February 11, 2024 13:55
dangeross
dangeross approved these changes Feb 11, 2024
View reviewed changes
Copy link
Contributor

@dangeross dangeross left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

It looks like setPaymentMetadata needs adding to sdk-flutter in the breez-sdk repo

@hydra-yse hydra-yse mentioned this pull request Feb 12, 2024
Merged
@hydra-yse hydra-yse requested a review from roeierez February 12, 2024 09:20
ok300
ok300 approved these changes Feb 29, 2024
View reviewed changes
Copy link
Contributor

@ok300 ok300 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

If you rebase on latest main, all CI tests should work.

@hydra-yse hydra-yse force-pushed the payment-metadata branch 2 times, most recently from 20f074f to 35941de Compare February 29, 2024 21:45
@hydra-yse hydra-yse force-pushed the payment-metadata branch 2 times, most recently from 4b71329 to e89e882 Compare February 29, 2024 22:21
@hydra-yse hydra-yse merged commit 6254560 into main Feb 29, 2024
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dangeross dangeross dangeross approved these changes

@ok300 ok300 ok300 approved these changes

@roeierez roeierez Awaiting requested review from roeierez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hydra-yse @ok300 @dangeross